Conversation
fetchUpstreamJson typed the parsed body as Record<string, unknown> but JSON.parse returns any, so an upstream response body of the literal `null` (or any JSON primitive) became null and crashed on `body.msg` with "Cannot read properties of null (reading 'msg')". Guard the parse result to only treat non-null objects as the body, otherwise fall back to wrapping the raw text as an error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughIn ChangesDefensive Upstream JSON Parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ea677daa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| typeof parsed === "object" && parsed !== null | ||
| ? (parsed as Record<string, unknown>) | ||
| : { |
There was a problem hiding this comment.
Preserve JSON string upload URLs
When AtlasCloud's media upload responds with a valid JSON string containing a URL, such as "https://...", this branch now wraps the parsed string in { error: { message: text } } instead of returning the string. uploadAtlasCloudMedia immediately passes this value to extractAtlasCloudUploadedMediaUrl, which has an explicit top-level string URL case, so those uploads regress from succeeding to AtlasCloud media upload did not return a usable URL; the null crash guard should not discard string primitives that existing extraction logic can handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/gateway/src/videos/videos.ts`:
- Around line 2719-2721: The object type guard in the parsed-body condition is
accepting arrays because typeof array returns "object" in JavaScript, which
causes array payloads to bypass the fallback handler and be returned as
Record<string, unknown>, violating the intended response contract. Modify the
guard expression that checks `typeof parsed === "object" && parsed !== null` to
additionally exclude arrays by adding an Array.isArray check, ensuring only
plain objects pass through while arrays fall through to the fallback case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d273d871-b5d3-4b38-9dd2-a76934ff05aa
📒 Files selected for processing (1)
apps/gateway/src/videos/videos.ts
| typeof parsed === "object" && parsed !== null | ||
| ? (parsed as Record<string, unknown>) | ||
| : { |
There was a problem hiding this comment.
Exclude arrays in the parsed-body guard to preserve response contract.
On Line 2719, the object check still accepts arrays (typeof [] === "object"), so array JSON payloads bypass the fallback and are returned as Record<string, unknown>. That contradicts the intended behavior for non-object payloads and can leak invalid body shapes downstream.
Suggested fix
- body =
- typeof parsed === "object" && parsed !== null
+ body =
+ typeof parsed === "object" &&
+ parsed !== null &&
+ !Array.isArray(parsed)
? (parsed as Record<string, unknown>)
: {
error: {
message: text,
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typeof parsed === "object" && parsed !== null | |
| ? (parsed as Record<string, unknown>) | |
| : { | |
| body = | |
| typeof parsed === "object" && | |
| parsed !== null && | |
| !Array.isArray(parsed) | |
| ? (parsed as Record<string, unknown>) | |
| : { | |
| error: { | |
| message: text, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/gateway/src/videos/videos.ts` around lines 2719 - 2721, The object type
guard in the parsed-body condition is accepting arrays because typeof array
returns "object" in JavaScript, which causes array payloads to bypass the
fallback handler and be returned as Record<string, unknown>, violating the
intended response contract. Modify the guard expression that checks `typeof
parsed === "object" && parsed !== null` to additionally exclude arrays by adding
an Array.isArray check, ensuring only plain objects pass through while arrays
fall through to the fallback case.
Problem
Production gateway crashed with:
fetchUpstreamJsonannotated the parsed response body asRecord<string, unknown>, butJSON.parsereturnsany. When an upstream provider responded with a body of the literal textnull(or any non-object JSON primitive),bodybecamenull, and the subsequenttypeof body.msgdereferencednulland threw.Fix
Guard the parse result: only treat a non-null
objectas the body. Anything else (null / primitive) falls back to wrapping the raw text in anerrorobject — the same path already used for non-JSON responses.Testing
pnpm turbo run build --filter=gatewaypassesSummary by CodeRabbit